Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
brandyscarney
left a comment
brandyscarney
left a comment
There was a problem hiding this comment.
I have added some questions / comments but I am approving this as you can tell me if you disagree on any or all.
| configs({ modes: ['ionic-md', 'md', 'ios'] }).forEach(({ title, screenshot, config }) => { | ||
| test.describe(title('title: basic'), () => { | ||
| test('should not have visual regressions', async ({ page }) => { | ||
| await page.goto('/src/components/title/test/basic', config); |
There was a problem hiding this comment.
We have been leaning toward migrating tests to page.setContent, is there a reason we aren't doing this here?
There was a problem hiding this comment.
Do we need an index.html preview for the color? I am fine if the answer is yes but we will probably need to add it for other components.
There was a problem hiding this comment.
No, removed in df0b85c. The test now renders with setContent.
| configs({ modes: ['ionic-md', 'md', 'ios'], directions: ['ltr'] }).forEach(({ title, screenshot, config }) => { | ||
| test.describe(title('title: color'), () => { | ||
| test('should apply the color to the title text', async ({ page }) => { | ||
| await page.goto('/src/components/title/test/color', config); |
There was a problem hiding this comment.
We have been leaning toward migrating tests to page.setContent, is there a reason we aren't doing this here?
See Breadcrumbs
I don't think we need to capture each color for each component. If we have proper screenshot tests in the theme folder capturing the colors then we should just need to do a few colors in the components themselves. Let me know your thoughts.
| }); | ||
| }); | ||
|
|
||
| describe('title: style event', () => { |
There was a problem hiding this comment.
Could we also cover this event being emit on the first connect?
Co-authored-by: Brandy Smith <brandyscarney@users.noreply.github.com>
5f572a1 to
b365e48
Compare
brandyscarney
left a comment
brandyscarney
left a comment
There was a problem hiding this comment.
This looks good just some minor test change requests!
There was a problem hiding this comment.
I think we should check at least 3 colors to make sure colors are being applied correctly. For example, if we ever decide that primary should be the default color (as it is in other components) this would no longer check if the color is being set properly. I would suggest primary, tertiary and one of the "light" colors like warning.
| ` | ||
| <style> | ||
| .ios .hide-ios, | ||
| .ionic .hide-ios { |
There was a problem hiding this comment.
Why is this class named hide-ios when it hides for both ios and ionic?
| } | ||
| </style> | ||
|
|
||
| <div id="header-wrapper"> |
There was a problem hiding this comment.
Could we rename header-wrapper to container? That's the id most e2e tests use for a wrapper element they screenshot: example
| <div id="header-wrapper"> | |
| <div id="container"> |
You would need to update it in all places & references to it.
This branch was successfully deployed
2 participants
Issue number: resolves internal
What is the current behavior?
No test sets
coloronion-title, and no test screenshots a title in the ionic theme in RTL.What is the new behavior?
colortest page and screenshots in the ionic, md and ios themes.basicscreenshots in RTL too. The ios and md RTL snapshots already existed without a test generating them; they now have one..toolbar-titleelement the iOS large title animations measure, and theionStyleevent.Does this introduce a breaking change?
Other information